-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate build to GitHub actions #1151
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! (assuming your builds all run successful, which is a big assumption).
All looks fine to me, I've left a few tiny optional comments, but you've definitely got this right.
Lovely code, I could read that all day
.github/workflows/event-consumer.yml
Outdated
- name: Compile, test and assembly | ||
run: sbt "project eventconsumer" "compile" "test" "assembly" | ||
|
||
- name: Upload to riff-raff | ||
uses: guardian/actions-riff-raff@v2 | ||
with: | ||
projectName: mobile-n10n:eventconsumer | ||
configPath: eventconsumer/riff-raff.yaml | ||
contentDirectories: | | ||
eventconsumer: | ||
- eventconsumer/target/scala-2.13/eventconsumer.jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes I've had differences between what ran locally and what came out in github actions, for the version of scala, ie. 2.12 locally vs. 2.13 in GHA, and those could change in theory.
Sometimes to fix this, I've added something like this:
- name: copy jar to root dir
run: cp eventconsumer/target/scala-*/eventconsumer.jar .
then in the riffraff action, provided the path at the root directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea - I've added this extra step :)
cd cdk | ||
npm install | ||
npm run lint | ||
npm test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're writing tests, might be nice to add a test reporter step after this.
This is the one I've used:
https://github.com/dorny/test-reporter/tree/main
and here's an example:
https://github.com/guardian/janus/blob/e41b96a9221c0f2cf5d558990d81b3d8dd981bbc/.github/workflows/build.yml#L55-L64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been looking at the test reports - for the scala tests we use specs2, which output test data in the .stats format. I don't think this is supported by the action. I tested anyway and got an expected error.
For the cdk tests, I can't see the output of the tests on my local machine, so not entirely sure what path to specify for the reporter. Will do a little more digging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely stuff! Thanks for doing this - this looks ginormous. Do we have a ticket to convert the non-cdk stuff in this project to cdk?
Yep, it's a bit big, sorry! About the cdk migration there's a ticket in the backlog https://theguardian.atlassian.net/browse/LIVE-4170 |
NB: these workflows make use of some repository secrets, I've made copies of the values of these in SSM:
|
Also switches from deprecated sbt-riffraff-artifact plugin to devx recommendation guardian/actions-riff-raff. Based on guardian/mobile-n10n#1151 and guardian/s3-upload#53
Migrate from Teamcity to Github actions Also switches from deprecated sbt-riffraff-artifact plugin to devx recommendation guardian/actions-riff-raff. Based on guardian/mobile-n10n#1151 and guardian/s3-upload#53
What does this change?
This PR adds GitHub workflows for the build of each of the apps inside in the n10n repo. Previously the builds were managed by TeamCity, but after this change is merged the corresponding TeamCity config will be deleted.
The current TeamCity configuration builds all projects, on every commit to every branch. The workflows in this PR change things slightly:
Each workflow follows one of these patterns:
assembly
command to generate a .jar.debian:packageBin
command (using pluginsbt-native-packager
) to generate a .deb.NB: not all projects have been converted to CDK, so the CDK synth step is not always required.
Docker lambdas
The
notificationworkerlambda
project deploys to docker lambdas, the historical reason for this is because the size of the .zip exceeded AWS's maximum size whereas docker lambdas don't impose such a limit.To use docker lambdas we need to upload images to a remote ECR repository. In order to push images to the repository we need to login to ECR. Previously we managed the docker auth/deploy using script, in the new workflow I've changed things slightly:
For a login to be successful we need to supply a custom role. I followed a couple of guides, which I'll link inline below, but my starting point was here. The custom role has been configured as following:
mobile
aws account for the GitHub token provider serviceI manually created the new role in the aws account, it's name is
mobile-n10n-deploy-role
. I believe I've applied the minimum permissions necessary for the workflow to succeed (including limiting the scope of any permissions applied to just the n10n ECR repository):ecr:GetAuthorizationToken
ecr:CompleteLayerUpload
,ecr:UploadLayerPart
,ecr:InitiateLayerUpload
,ecr:BatchCheckLayerAvailability
,ecr:PutImage
Given login was successful, the workflow compiles and tests the code and then uses the docker
publishLocal
command to generate a local image.A small change I had to make was to explicitly tag the local image using the expected naming convention (
$repository-url:$build-number
). I'm not quite sure how this was working in the TeamCity build, but this current solution does work. If we migrated to using adocker build
command, I think we can explicitly set a tag value, but I couldn't see anything obvious/immediate in the packager docs.Given a local docker image exists with the expected tag we can then push the image to the remote ECR repository.
Testing